Harden Linux manager executable lookup to prevent PATH hijacking#4621
Conversation
Gabriel Dufresne (GabrielDuf)
deleted the
codex/fix-path-hijacking-vulnerability-in-linux-managers
branch
Gabriel Dufresne (GabrielDuf)
restored the
codex/fix-path-hijacking-vulnerability-in-linux-managers
branch
There was a problem hiding this comment.
Pull request overview
This PR hardens Linux package manager executable discovery (apt/dnf/pacman) to avoid selecting binaries from the user-controlled PATH that may later be executed under elevation (sudo/pkexec).
Changes:
- Replace
CoreTools.WhichMultiple(...)-based discovery with fixed, trusted absolute paths foraptandpacman. - Replace
CoreTools.WhichMultiple(...)-based discovery with fixed, trusted absolute paths fordnf/dnf5.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/UniGetUI.PackageEngine.Managers.Apt/Apt.cs | Restricts apt candidate lookup to /usr/bin/apt and /bin/apt. |
| src/UniGetUI.PackageEngine.Managers.Dnf/Dnf.cs | Restricts dnf/dnf5 candidate lookup to /usr/bin/* and /bin/*. |
| src/UniGetUI.PackageEngine.Managers.Pacman/Pacman.cs | Restricts pacman candidate lookup to /usr/bin/pacman and /bin/pacman. |
Comments suppressed due to low confidence (1)
src/UniGetUI.PackageEngine.Managers.Dnf/Dnf.cs:63
- This candidate list uses File.Exists only; on Linux this can include non-executable files and cause the manager to be considered available even though the binary can’t be executed. Add an executability check (execute bits) or use CoreTools.WhichMultiple/Which with absolute paths to validate executability without relying on PATH search.
var candidates = new List<string>();
foreach (var path in new[] { "/usr/bin/dnf5", "/usr/bin/dnf", "/bin/dnf5", "/bin/dnf" })
{
if (File.Exists(path) && !candidates.Contains(path))
candidates.Add(path);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var candidates = new List<string>(); | ||
| foreach (var path in new[] { "/usr/bin/pacman", "/bin/pacman" }) | ||
| { | ||
| if (File.Exists(path) && !candidates.Contains(path)) | ||
| candidates.Add(path); |
There was a problem hiding this comment.
FindCandidateExecutableFiles currently accepts any existing file at these locations, even if it isn’t executable (no +x). Consider checking Unix execute permissions (or using CoreTools.WhichMultiple/Which with absolute paths) so the manager isn’t marked found when the binary can’t be launched.
| var candidates = new List<string>(); | ||
| foreach (var path in new[] { "/usr/bin/apt", "/bin/apt" }) | ||
| { | ||
| if (File.Exists(path) && !candidates.Contains(path)) | ||
| candidates.Add(path); |
There was a problem hiding this comment.
Candidate discovery only checks File.Exists; it will treat non-executable files (missing +x) as valid candidates and may mark the manager as found, then later fail to launch with a permission error. Consider additionally verifying executability (e.g., via Unix execute bits) or reusing CoreTools.WhichMultiple/Which on the absolute paths to leverage its executable-checking logic without searching PATH.
Motivation
apt,dnf, andpacmanbinaries were discovered from the userPATHand later invoked under an elevator (sudo/pkexec), which allowed a PATH-hijacked binary to be run as root after user approval.PATHinfluence.Description
FindCandidateExecutableFiles()insrc/UniGetUI.PackageEngine.Managers.Apt/Apt.csto only consider/usr/bin/aptand/bin/aptinstead of usingCoreTools.WhichMultiple("apt").FindCandidateExecutableFiles()insrc/UniGetUI.PackageEngine.Managers.Dnf/Dnf.csto only consider/usr/bin/dnf5,/usr/bin/dnf,/bin/dnf5, and/bin/dnfand removedWhichMultipleusage fordnfcandidates.FindCandidateExecutableFiles()insrc/UniGetUI.PackageEngine.Managers.Pacman/Pacman.csto only consider/usr/bin/pacmanand/bin/pacmaninstead of usingCoreTools.WhichMultiple("pacman").Testing
dotnet --versionreturnedcommand not foundsodotnet testcould not be run.Codex Task